Skip to content

feat(settings): implement Settings view with config.toml persistence#22

Merged
mpiton merged 7 commits intomainfrom
feat/23-settings
Apr 11, 2026
Merged

feat(settings): implement Settings view with config.toml persistence#22
mpiton merged 7 commits intomainfrom
feat/23-settings

Conversation

@mpiton
Copy link
Copy Markdown
Owner

@mpiton mpiton commented Apr 11, 2026

Summary

  • Expand AppConfig domain model from 9 to 32 fields covering 6 settings categories (General, Downloads, Network, Remote Access, Browser Integration, Appearance)
  • Create TomlConfigStore adapter for reading/writing ~/.config/vortex/config.toml with atomic writes, auto-defaults, and forward-compatible deserialization
  • Implement UpdateConfigCommand handler with input validation (proxy_type, theme, port bounds, concurrent/segment limits, timeout range)
  • Add settings_get / settings_update Tauri IPC commands with camelCase DTO serialization
  • Build SettingsView with 6-tab sidebar layout and shared SettingToggle / SettingNumberInput components
  • Implement all 6 section components: GeneralSection, DownloadsSection, NetworkSection, RemoteAccessSection (with API key display), BrowserSection, AppearanceSection (with accent color presets)
  • Add event-based cache invalidation via settings-updated Tauri event listener

Test plan

  • 368 Rust tests pass (including 8 TOML adapter tests + 4 validation tests)
  • 280 TypeScript tests pass (including 35 settings tests)
  • cargo clippy -- -D warnings clean
  • tsc --noEmit clean
  • oxlint 0 errors
  • Lefthook pre-commit hooks pass (secrets, fmt, lint, clippy)
  • Manual: Open Settings, switch tabs, change values, verify persistence after reload
  • Manual: Change theme/accent color, verify UI updates
  • Manual: Enable REST API, verify API key display/copy/regenerate

Summary by cubic

Implements the full Settings experience with persistent config.toml, a 6‑tab UI, strict post‑merge validation, and concurrency‑safe config access, completing Task 23. Also adds tauri-plugin-pilot on Unix and isolates its dev-pilot capability to Linux/macOS to keep Windows CI green.

  • New Features

    • Expanded AppConfig to 32 fields across General, Downloads, Network, Remote Access, Browser, and Appearance.
    • TomlConfigStore persistence to ~/.config/vortex/config.toml with atomic writes and sensible defaults.
    • Validated UpdateConfigCommand and IPC: settings_get / settings_update (camelCase DTOs, { patch: ... }) plus SettingsUpdated event; cross‑field checks (e.g., require proxyUrl when proxyTypenone).
    • SettingsView with 6 tabs and shared inputs; API key show/copy/regenerate, accent color presets, and cache invalidation on settings-updated. Adds tauri-plugin-pilot (Unix‑only) with dev-pilot capability restricted to Linux/macOS.
  • Bug Fixes

    • Concurrency: guarded reads and writes with a Mutex for safe config access.
    • Persistence/security: temp config.toml created with 0o600 permissions and sync_all used for crash‑safe atomic writes.
    • IPC/UX: fixed { patch: ... } IPC shape; buffered inputs commit on blur and sync after clamping; controlled Browser inputs; disabled Browse until picker is wired.
    • Accessibility/clarity: added aria-pressed, htmlFor/id, aria-label on Theme/Language selects; clarified speed limit to MiB/s; numeric fields respect step when committing.
    • CI: moved pilot:default permissions into a platform‑scoped dev-pilot capability file to avoid Windows build validation errors.

Written for commit 146f4a2. Summary will update on new commits.

Summary by CodeRabbit

  • New Features

    • Full Settings UI with tabs: General, Downloads, Network, Remote Access, Browser, Appearance.
    • Configure proxy, download limits, timeouts, web interface port, REST/WebSocket/API key (show/copy/regenerate), theme, accent color, compact mode, and many other preferences.
    • Settings persist safely on disk and update immediately across the app; external changes auto-refresh the UI.
  • Tests

    • Added comprehensive frontend tests covering the Settings view and all sections.

Full-stack Settings implementation (Task 23):

Backend:
- Expand AppConfig from 9 to 32 fields (General, Downloads, Network,
  Remote Access, Browser Integration, Appearance)
- TomlConfigStore adapter with atomic writes and auto-defaults
- UpdateConfigCommand handler with input validation
- settings_get/settings_update IPC commands with camelCase DTOs

Frontend:
- SettingsView with 6-tab sidebar layout
- GeneralSection, DownloadsSection, NetworkSection, RemoteAccessSection,
  BrowserSection, AppearanceSection components
- Event-based cache invalidation for external config changes
- 35 component tests + 7 store tests
@github-actions github-actions bot added documentation Improvements or additions to documentation rust frontend ui labels Apr 11, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 11, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a full settings system: expanded AppConfig and ConfigPatch with apply_patch, TOML-backed TomlConfigStore with atomic writes, UpdateConfigCommand with validation and SettingsUpdated event, Tauri IPC (settings_get/settings_update) and a tabbed React SettingsView with six sections, shared controls, and tests.

Changes

Cohort / File(s) Summary
Domain model & patching
src-tauri/src/domain/model/config.rs, src-tauri/src/domain/event.rs
Expanded AppConfig (~9→32 fields), updated ConfigPatch, added apply_patch(), and new DomainEvent::SettingsUpdated.
Config persistence adapter
src-tauri/src/adapters/driven/config/mod.rs, src-tauri/src/adapters/driven/config/toml_config_store.rs
Added TomlConfigStore (PathBuf-backed, Mutex, ConfigDto <-> AppConfig conversions, snake_case serde, parent-dir creation, atomic tmp+rename writes) and public re-export; includes unit tests for defaulting/roundtrip/patching.
Application commands & bus
src-tauri/src/application/commands/update_config.rs, src-tauri/src/application/commands/mod.rs, src-tauri/src/application/command_bus.rs
New update-config handler: merges via apply_patch, validates fields and cross-field rules, persists via config store, publishes SettingsUpdated; command registration updated and mocks delegate to apply_patch.
Tauri IPC & bridge
src-tauri/src/adapters/driving/tauri_ipc.rs, src-tauri/src/adapters/driven/event/tauri_bridge.rs, src-tauri/src/lib.rs
Added settings_get/settings_update IPC handlers with camelCase DTOs and nullable patch semantics, mapped SettingsUpdated"settings-updated" event, exported handlers and TomlConfigStore.
Frontend types & store
src/types/settings.ts, src/stores/settingsStore.ts, src/stores/__tests__/settingsStore.test.ts
New TypeScript AppConfig, AppConfigPatch, unions and ACCENT_PRESETS; store now imports types and invokes settings_update as tauriInvoke('settings_update', { patch }).
Settings UI & components
src/views/SettingsView/SettingsView.tsx, src/views/SettingsView/*Section.tsx, src/views/SettingsView/SettingField.tsx, src/views/SettingsView/index.ts, (removed) src/views/SettingsView.tsx
Replaced placeholder view with tabbed SettingsView, six section components (General, Downloads, Network, Remote Access, Browser, Appearance), reusable SettingToggle/SettingNumberInput, React Query + Tauri hooks, and event-driven cache invalidation.
Tests & test adjustments
src/views/SettingsView/__tests__/*, src/components/__tests__/ClipboardIndicator.test.tsx, src/stores/__tests__/settingsStore.test.ts, src-tauri/.../ports/driven/tests.rs
Added ~35+ frontend tests for the view and sections; updated tests and in-memory/mock stores to use apply_patch and to the new config shape.
Build & config files
CHANGELOG.md, src-tauri/Cargo.toml, src-tauri/capabilities/default.json
Changelog updated; added tauri-plugin-pilot dependency and "pilot:default" capability permission.

Sequence Diagram(s)

sequenceDiagram
    participant FE as Frontend (React)
    participant IPC as Tauri IPC
    participant Cmd as CommandBus
    participant Store as TomlConfigStore
    participant FS as Filesystem
    participant EB as EventBus

    FE->>IPC: invoke 'settings_update' { patch }
    IPC->>Cmd: handle_update_config(UpdateConfigCommand)
    Cmd->>Cmd: merge (apply_patch) & validate config
    Cmd->>Store: update_config(ConfigPatch)
    Store->>FS: read config.toml (or use default)
    FS-->>Store: ConfigDto
    Store->>Store: apply_patch(config, patch)
    Store->>FS: write config.toml.tmp (pretty TOML)
    FS-->>Store: OK
    Store->>FS: rename .tmp -> config.toml (atomic)
    FS-->>Store: OK
    Store-->>Cmd: return updated AppConfig
    Cmd->>EB: publish DomainEvent::SettingsUpdated
    EB-->>FE: tauri event "settings-updated"
    Cmd-->>IPC: return SettingsDto
    IPC-->>FE: Result<SettingsDto>
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

Suggested labels

configuration, dependencies

Poem

🐰 Hopping through settings, toggles aglow,
TOML files whisper the preferences we know,
Tabs click and update, mutations take flight,
Events give a nudge — the UI stays right,
Rabbits cheer softly: configurations grow.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 29.07% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately and concisely summarizes the primary change: implementing a Settings view with config.toml persistence. It directly corresponds to the main feature addition.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/23-settings

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Apr 11, 2026

Greptile Summary

This PR adds a full Settings view with 6 tabs, a TomlConfigStore with atomic file writes, an UpdateConfigCommand with input validation, and camelCase IPC DTOs — a solid, well-tested foundation. Three P1 issues need attention before merge:

  • Optimistic update not rolled back (settingsStore.ts): failed IPC writes leave the UI permanently out of sync with the persisted config.
  • Stale textareas in BrowserSection (BrowserSection.tsx): defaultValue on the excluded-domains/extensions textareas means they silently display old data after a config refetch.
  • Missing SettingsUpdated event in clipboard toggle (toggle_clipboard.rs): the comment in SettingsView.tsx relies on settings-updated to invalidate the cache on external clipboard changes, but handle_toggle_clipboard never publishes that event.

Confidence Score: 4/5

Three P1 defects (no optimistic rollback, stale uncontrolled textareas, missing SettingsUpdated event from clipboard toggle) should be fixed before merge.

The Rust backend (TOML store, validation, IPC layer) is clean and well-tested. The three P1 issues are all in the frontend store/component layer and are straightforward to fix; none require architectural changes.

src/stores/settingsStore.ts, src/views/SettingsView/BrowserSection.tsx, src-tauri/src/application/commands/toggle_clipboard.rs

Important Files Changed

Filename Overview
src/stores/settingsStore.ts Optimistic update is applied before IPC but never rolled back on failure; UI permanently shows rejected state after any storage error
src-tauri/src/application/commands/toggle_clipboard.rs Updates config store and starts/stops observer correctly, but never publishes DomainEvent::SettingsUpdated, so the Settings view cache is not invalidated after a tray-based clipboard toggle
src/views/SettingsView/BrowserSection.tsx Excluded domains/extensions textareas use defaultValue (uncontrolled), so they won't reflect config changes from external refetches
src-tauri/src/adapters/driven/config/toml_config_store.rs Atomic write-via-rename pattern is correct; serde default deserialization gives good forward compatibility; comprehensive test coverage
src-tauri/src/application/commands/update_config.rs Validation logic is correct and well-tested; SettingsUpdated event is published after every successful update
src-tauri/src/adapters/driving/tauri_ipc.rs Settings DTO layer is clean; camelCase serialization matches TypeScript types; settings_get/settings_update IPC commands correctly delegate to CommandBus
src/views/SettingsView/SettingsView.tsx Tab layout, query invalidation on settings-updated event, and loading skeleton are all well-structured; listener cleanup in useEffect is correct
src/views/SettingsView/RemoteAccessSection.tsx API key show/hide, copy, and frontend UUID regeneration are implemented correctly; security warning card is a nice UX touch
src/views/SettingsView/SettingField.tsx Shared toggle and number-input components are clean; number input fires IPC on every keystroke (P2 style concern)
src-tauri/src/domain/model/config.rs AppConfig, ConfigPatch, and apply_patch are well-designed; Option-of-Option semantics for nullable fields are clearly documented

Sequence Diagram

sequenceDiagram
    participant UI as Section Component
    participant Store as settingsStore
    participant IPC as Tauri IPC
    participant Bus as CommandBus
    participant TOML as TomlConfigStore
    participant EB as EventBus
    participant SV as SettingsView

    UI->>Store: updateConfig(partial)
    Store->>Store: optimistic patch applied
    Store->>IPC: tauriInvoke('settings_update', patch)
    IPC->>Bus: handle_update_config(cmd)
    Bus->>Bus: validate_patch()
    Bus->>TOML: update_config(patch)
    TOML->>TOML: read_or_default()
    TOML->>TOML: apply_patch()
    TOML->>TOML: write .tmp then rename (atomic)
    TOML-->>Bus: AppConfig
    Bus->>EB: publish(SettingsUpdated)
    EB->>SV: emit settings-updated
    SV->>IPC: invalidate settings_get query
    IPC->>TOML: get_config()
    TOML-->>SV: fresh AppConfig

    note over Store,IPC: On IPC error optimistic state is NOT rolled back
    note over Bus,EB: handle_toggle_clipboard skips this publish step
Loading

Comments Outside Diff (2)

  1. src/stores/settingsStore.ts, line 22-35 (link)

    P1 Optimistic update never rolled back on IPC failure

    The config is patched optimistically before the IPC call, but on error the rollback never happens — set({ error: message }) only sets the error string. If the backend rejects the write (e.g., disk full, permission error), the UI permanently shows the rejected value while the persisted config stays unchanged. The user would need to reload to see the real state.

    Fix: snapshot previous before applying the optimistic patch and restore it in the catch block: set({ error: message, config: previous }).

    Fix in Claude Code

  2. src-tauri/src/application/commands/toggle_clipboard.rs, line 6-30 (link)

    P1 handle_toggle_clipboard never publishes SettingsUpdated

    SettingsView.tsx registers a settings-updated listener specifically to "invalidate cache when settings change from another source (e.g. clipboard toggle)". However, handle_toggle_clipboard calls config_store().update_config(...) directly — it never publishes DomainEvent::SettingsUpdated to the event bus. As a result, toggling clipboard monitoring from the tray icon or ClipboardIndicator will NOT cause the Settings view to reflect the updated state. Add self.event_bus().publish(DomainEvent::SettingsUpdated); after the successful observer toggle.

    Fix in Claude Code

Fix All in Claude Code

Reviews (1): Last reviewed commit: "feat(settings): implement Settings view ..." | Re-trigger Greptile

Comment on lines +37 to +65
<p className="text-xs text-muted-foreground">Comma-separated list of domains to ignore</p>
<textarea
className="h-20 w-full rounded-md border border-input bg-transparent px-3 py-2 text-sm shadow-xs outline-none focus-visible:border-ring focus-visible:ring-[3px] focus-visible:ring-ring/50 dark:bg-input/30"
defaultValue={config.excludedDomains.join(', ')}
onBlur={(e) => {
const domains = e.target.value
.split(',')
.map((d) => d.trim())
.filter(Boolean);
handleChange('excludedDomains', domains);
}}
/>
</div>

<div className="space-y-1">
<p className="text-sm font-medium">Excluded extensions</p>
<p className="text-xs text-muted-foreground">Comma-separated list of file extensions to ignore</p>
<textarea
className="h-20 w-full rounded-md border border-input bg-transparent px-3 py-2 text-sm shadow-xs outline-none focus-visible:border-ring focus-visible:ring-[3px] focus-visible:ring-ring/50 dark:bg-input/30"
defaultValue={config.excludedExtensions.join(', ')}
onBlur={(e) => {
const extensions = e.target.value
.split(',')
.map((ext) => ext.trim())
.filter(Boolean);
handleChange('excludedExtensions', extensions);
}}
/>
</div>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Uncontrolled defaultValue causes stale textarea after config refetch

Both textareas use defaultValue, making them uncontrolled inputs. React only reads defaultValue on mount; if the parent config prop changes (e.g., after a settings-updated event triggers a settings_get refetch), the textareas will silently keep displaying the old values. The simplest fix is to add a key prop derived from the current list value so React remounts the element when the config is externally updated — this keeps the onBlur-based save pattern without adding per-keystroke IPC calls. The same issue applies to both the excluded-domains and excluded-extensions textareas.

Fix in Claude Code

Comment on lines +48 to +56
<Input
type="number"
className="w-24"
value={value}
min={min}
max={max}
step={step}
onChange={(e) => onChange(Number(e.target.value))}
/>
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Number input fires IPC on every keystroke

onChange fires on each keystroke, so typing "100" sends three separate IPC calls (1, 10, 100) each triggering an atomic file write. Consider using onBlur (consistent with the textarea pattern in BrowserSection) or debouncing the handler to coalesce rapid changes into a single write.

Fix in Claude Code

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

9 issues found across 30 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src/views/SettingsView/NetworkSection.tsx">

<violation number="1" location="src/views/SettingsView/NetworkSection.tsx:57">
P2: This controlled text field saves on every keystroke without local draft state, which can make typing laggy/unreliable and causes excessive IPC/disk writes. Commit on blur (or debounce with local state) instead.</violation>
</file>

<file name="src-tauri/src/adapters/driving/tauri_ipc.rs">

<violation number="1" location="src-tauri/src/adapters/driving/tauri_ipc.rs:537">
P1: `settings_update` expects a `patch`-named argument, but callers send patch fields at the top level, causing IPC argument mismatch and failed settings updates.</violation>
</file>

<file name="src/views/SettingsView/SettingField.tsx">

<violation number="1" location="src/views/SettingsView/SettingField.tsx:55">
P2: This numeric input persists changes on every keystroke, causing repeated `settings_update` IPC calls and config file writes while the user is still typing. Buffer the edit locally and commit on blur (or debounce) to avoid write amplification.</violation>

<violation number="2" location="src/views/SettingsView/SettingField.tsx:55">
P2: Using `Number(e.target.value)` on a number input coerces empty input to `0`, causing accidental config updates when users clear the field.</violation>
</file>

<file name="src/views/SettingsView/GeneralSection.tsx">

<violation number="1" location="src/views/SettingsView/GeneralSection.tsx:37">
P2: The “Browse” control is non-functional (no click handler), and the directory field is read-only, so `downloadDir` cannot be changed from this screen.</violation>
</file>

<file name="src-tauri/src/lib.rs">

<violation number="1" location="src-tauri/src/lib.rs:82">
P1: `settings_get`/`settings_update` are registered before `AppState` is managed, so settings IPC calls will fail at runtime.</violation>
</file>

<file name="src/views/SettingsView/BrowserSection.tsx">

<violation number="1" location="src/views/SettingsView/BrowserSection.tsx:40">
P2: Using `defaultValue` here makes the field stale after `config` refreshes; subsequent blur events can overwrite newer settings with outdated text.</violation>
</file>

<file name="src-tauri/src/adapters/driven/config/toml_config_store.rs">

<violation number="1" location="src-tauri/src/adapters/driven/config/toml_config_store.rs:44">
P1: The config file contains `api_key`, but the temp file is written without explicitly restrictive permissions. This can expose the API key to other local users on permissive filesystem defaults.</violation>

<violation number="2" location="src-tauri/src/adapters/driven/config/toml_config_store.rs:63">
P1: `update_config` is not concurrency-safe: concurrent settings updates can race and lose writes due to unsynchronized read-modify-write on the same file.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

#[tauri::command]
pub async fn settings_update(
state: State<'_, AppState>,
patch: ConfigPatchDto,
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai bot Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: settings_update expects a patch-named argument, but callers send patch fields at the top level, causing IPC argument mismatch and failed settings updates.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src-tauri/src/adapters/driving/tauri_ipc.rs, line 537:

<comment>`settings_update` expects a `patch`-named argument, but callers send patch fields at the top level, causing IPC argument mismatch and failed settings updates.</comment>

<file context>
@@ -342,6 +343,211 @@ pub async fn clipboard_state(state: State<'_, AppState>) -> Result<bool, String>
+#[tauri::command]
+pub async fn settings_update(
+    state: State<'_, AppState>,
+    patch: ConfigPatchDto,
+) -> Result<SettingsDto, String> {
+    let cmd = UpdateConfigCommand {
</file context>
Fix with Cubic

link_resolve,
clipboard_toggle,
clipboard_state,
settings_get,
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai bot Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: settings_get/settings_update are registered before AppState is managed, so settings IPC calls will fail at runtime.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src-tauri/src/lib.rs, line 82:

<comment>`settings_get`/`settings_update` are registered before `AppState` is managed, so settings IPC calls will fail at runtime.</comment>

<file context>
@@ -77,6 +79,8 @@ pub fn run() {
             link_resolve,
             clipboard_toggle,
             clipboard_state,
+            settings_get,
+            settings_update,
         ])
</file context>
Fix with Cubic

placeholder="Default download directory"
className="flex-1"
/>
<Button variant="outline" size="icon" aria-label="Browse">
Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai bot Apr 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: The “Browse” control is non-functional (no click handler), and the directory field is read-only, so downloadDir cannot be changed from this screen.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/views/SettingsView/GeneralSection.tsx, line 37:

<comment>The “Browse” control is non-functional (no click handler), and the directory field is read-only, so `downloadDir` cannot be changed from this screen.</comment>

<file context>
@@ -0,0 +1,89 @@
+            placeholder="Default download directory"
+            className="flex-1"
+          />
+          <Button variant="outline" size="icon" aria-label="Browse">
+            <FolderOpen className="size-4" />
+          </Button>
</file context>
Fix with Cubic

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 11

🧹 Nitpick comments (2)
src-tauri/src/adapters/driven/event/tauri_bridge.rs (1)

37-37: Add a unit test for the new settings-updated mapping contract.

Nice mapping, but this new branch is currently unguarded in this file’s tests. A small test asserting name and {} payload will prevent accidental regressions.

Suggested test addition
 #[test]
 fn test_event_payload_clipboard_url_detected() {
@@
 }
+
+#[test]
+fn test_settings_updated_event_bridge_mapping() {
+    let event = DomainEvent::SettingsUpdated;
+    let (name, payload) = to_tauri_event(&event);
+    assert_eq!(name, "settings-updated");
+    assert_eq!(payload, json!({}));
+}

Also applies to: 90-90

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src-tauri/src/adapters/driven/event/tauri_bridge.rs` at line 37, Add a unit
test to cover the new DomainEvent::SettingsUpdated mapping in tauri_bridge.rs:
locate the match arm that maps DomainEvent::SettingsUpdated =>
"settings-updated" (e.g., in the function that converts DomainEvent to a Tauri
event name/payload) and add a test that constructs DomainEvent::SettingsUpdated,
invokes the conversion function (or the public API that produces the (name,
payload) pair), and asserts the name equals "settings-updated" and the payload
equals an empty object ({}). Ensure the test is placed alongside existing tests
in the same module and follows the project’s test conventions so this mapping is
guarded against regressions.
src/views/SettingsView/__tests__/SettingsView.test.tsx (1)

14-16: Assert the exact settings-updated subscription.

This mock accepts any listen(...) call, so a typo in the event name would still pass even though the backend bridge emits a single "settings-updated" event. Please assert the subscribed name explicitly.

Small tightening
+const mockListen = vi.hoisted(() => vi.fn().mockResolvedValue(vi.fn()));
+
 vi.mock('@tauri-apps/api/event', () => ({
-  listen: vi.fn().mockResolvedValue(vi.fn()),
+  listen: mockListen,
 }));
   it('should render all 6 tab buttons', async () => {
     renderWithProviders();
 
     await waitFor(() => {
       expect(screen.getByRole('button', { name: /General/ })).toBeInTheDocument();
     });
+
+    expect(mockListen).toHaveBeenCalledWith('settings-updated', expect.any(Function));
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/views/SettingsView/__tests__/SettingsView.test.tsx` around lines 14 - 16,
Update the mock for '@tauri-apps/api/event' in SettingsView.test.tsx so the
mocked listen call asserts the exact event name; replace the current looser
vi.mock that returns a generic listen with a mockImplementation on listen that
checks its first argument equals "settings-updated" (and then returns a resolved
cleanup function), referencing the existing vi.mock(...) and listen mock to
locate where to change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src-tauri/src/adapters/driven/config/toml_config_store.rs`:
- Around line 62-66: The update_config read-modify-write in update_config
currently has no synchronization and can be lost by concurrent callers; wrap the
entire sequence (read_or_default -> apply_patch -> write_config) with a durable
lock (e.g., a process-level file lock or a Mutex stored on the config adapter)
to serialize transactions. Locate update_config, read_or_default, write_config,
and apply_patch and acquire the lock before calling read_or_default and hold it
until after write_config returns (release on success/error); prefer a file lock
(flock or fs2) for cross-process safety, or an internal Mutex for in-process
only, and ensure errors while locking are converted to DomainError.

In `@src-tauri/src/adapters/driving/tauri_ipc.rs`:
- Around line 377-382: The SettingsDto currently includes the raw api_key field
(see fields
web_interface_enabled/web_interface_port/rest_api_enabled/api_key/websocket_enabled)
and settings_get/settings_update serialize and return it to the renderer; change
SettingsDto so it does NOT expose the secret directly: remove or replace the
api_key string with a masked placeholder (e.g. "*****") or a boolean hasApiKey
flag, and update any code paths in settings_get and settings_update (and related
code around the other occurrences you flagged) to return the masked/flagged
value instead; implement a separate explicit IPC command (e.g. reveal_api_key /
regenerate_api_key / copy_api_key) that performs secure server-side
retrieval/regeneration and only returns the real secret when authorized.

In `@src-tauri/src/application/commands/update_config.rs`:
- Around line 10-63: validate_patch currently only checks the incoming
ConfigPatch fields, letting invalid combined state (e.g. proxy_type set to
"http"/"socks5" while proxy_url is empty) be persisted; update
handle_update_config to merge cmd.patch with the current config from
config_store() to produce the post-merge AppConfig, validate that merged config
before calling config_store().update_config, and only persist if validation
passes (either add a new validate_config(AppConfig) or adapt validate_patch to
accept/validate the fully-merged config); reference validate_patch,
handle_update_config, config_store().update_config, Cmd.patch and AppConfig when
locating the changes.

In `@src-tauri/src/lib.rs`:
- Around line 82-83: Create and manage the AppState instance before registering
the settings IPC handlers: instantiate your AppState (ensuring it meets Send +
Sync requirements or is wrapped in Arc/Mutex as needed) and call the builder's
manage method (app.manage(state) / builder.manage(state)) prior to registering
settings_get and settings_update so the handlers receiving State<'_, AppState>
have a valid managed state at runtime; update the init sequence to construct the
state and call manage(...) before the handler registration that references
settings_get and settings_update.

In `@src/views/SettingsView/AppearanceSection.tsx`:
- Around line 63-75: The accent swatch buttons only use visual border styling to
indicate selection; update the button elements rendered from ACCENT_PRESETS in
AppearanceSection (where handleChange is called and compared to
config.accentColor) to expose the active state to assistive tech by adding a
proper accessible state — e.g., set aria-pressed={config.accentColor ===
preset.value} or convert the group to a radio role with role="radiogroup" on the
container and role="radio" plus aria-checked on each item — and ensure keyboard
activation still calls handleChange('accentColor', preset.value).

In `@src/views/SettingsView/BrowserSection.tsx`:
- Around line 38-64: The textareas in BrowserSection use defaultValue so they
don't update when config changes; replace them with controlled inputs: add
useState hooks (e.g., excludedDomainsText, excludedExtensionsText) initialized
from config.excludedDomains.join(', ') / config.excludedExtensions.join(', '),
add a useEffect that updates those state strings when config changes, set each
textarea's value to the corresponding state, update state in onChange, and keep
the existing onBlur parsing to call handleChange('excludedDomains', ...) and
handleChange('excludedExtensions', ...). This mirrors the controlled pattern
used in GeneralSection and ensures the UI stays in sync with config updates.

In `@src/views/SettingsView/GeneralSection.tsx`:
- Around line 37-39: The Browse Button renders an interactive control with no
action; either wire it to a folder-picker handler or make it non-interactive
until implemented. Add an onClick handler (e.g., handleBrowseFolder) on the
Button (the one rendering FolderOpen) that opens a hidden file input configured
for directory selection (input type="file" with webkitdirectory / directory
attributes, or call the platform-specific API if running in Electron) and handle
the selected path(s) in your component state, or alternatively set the Button's
disabled prop and update any accessibility labels to reflect the disabled state
until the folder selection feature is implemented.

In `@src/views/SettingsView/NetworkSection.tsx`:
- Around line 56-69: The User agent and proxy inputs currently write to config
on every keystroke (Input components using value={config.userAgent} and
value={config.proxyUrl} with onChange calling handleChange -> mutate), causing
an IPC/config write per character; change these to use local draft state (e.g.,
useState hooks like proxyDraft and userAgentDraft) bound to the Input values and
update drafts on onChange, then call handleChange/mutate only on onBlur (or
after a debounced timeout or an explicit Save/Apply button) to persist changes;
update the Input handlers and the component initialization to seed drafts from
config and ensure the blur/debounce/save path calls the existing
handleChange('proxyUrl', ...) and handleChange('userAgent', ...) so persistence
logic stays centralized.

In `@src/views/SettingsView/SettingField.tsx`:
- Around line 48-55: The input currently calls onChange(Number(...)) on every
keystroke; change SettingField to buffer edits in a local state (e.g., const
[localValue, setLocalValue] = useState(value) with a useEffect to sync incoming
value), bind the Input value to localValue and update it in onChange
(setLocalValue), then call the external onChange only when the user commits
(onBlur and onKeyDown for Enter) converting and validating the value against
min/max/step before calling onChange(Number(localValue)); keep the prop names
value, onChange, min, max, step and the Input component usage to locate changes.
- Around line 13-18: The visible label/description text in SettingField is not
programmatically associated with the form control; update the component to
generate or accept a stable id (useId or an id prop) and pass that id to the
Switch and Input components, set the label wrapper to use htmlFor or
aria-labelledby pointing to that id, and add aria-describedby linking to the
description element (if present); ensure you update both the checkbox row
(Switch, checked/onCheckedChange) and the input row (Input, value/onChange) so
clicking the text focuses/toggles the control and screen readers read the
description.

In `@src/views/SettingsView/SettingsView.tsx`:
- Around line 63-77: The current check conflates loading and error: replace the
combined condition so only isLoading renders the skeletons and handle a missing
config as an error state; when config is falsy and isLoading is false render an
error UI with a message and a retry control that calls the same settings_get (or
the existing fetch/settings loader) to re-attempt the request, and ensure any
error details from the failed settings_get are surfaced to the user or stored in
state for display.

---

Nitpick comments:
In `@src-tauri/src/adapters/driven/event/tauri_bridge.rs`:
- Line 37: Add a unit test to cover the new DomainEvent::SettingsUpdated mapping
in tauri_bridge.rs: locate the match arm that maps DomainEvent::SettingsUpdated
=> "settings-updated" (e.g., in the function that converts DomainEvent to a
Tauri event name/payload) and add a test that constructs
DomainEvent::SettingsUpdated, invokes the conversion function (or the public API
that produces the (name, payload) pair), and asserts the name equals
"settings-updated" and the payload equals an empty object ({}). Ensure the test
is placed alongside existing tests in the same module and follows the project’s
test conventions so this mapping is guarded against regressions.

In `@src/views/SettingsView/__tests__/SettingsView.test.tsx`:
- Around line 14-16: Update the mock for '@tauri-apps/api/event' in
SettingsView.test.tsx so the mocked listen call asserts the exact event name;
replace the current looser vi.mock that returns a generic listen with a
mockImplementation on listen that checks its first argument equals
"settings-updated" (and then returns a resolved cleanup function), referencing
the existing vi.mock(...) and listen mock to locate where to change.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 51a4c155-7801-4034-9476-439b3e9d20ac

📥 Commits

Reviewing files that changed from the base of the PR and between c33a164 and 6ff5002.

📒 Files selected for processing (30)
  • CHANGELOG.md
  • src-tauri/src/adapters/driven/config/mod.rs
  • src-tauri/src/adapters/driven/config/toml_config_store.rs
  • src-tauri/src/adapters/driven/event/tauri_bridge.rs
  • src-tauri/src/adapters/driven/mod.rs
  • src-tauri/src/adapters/driving/tauri_ipc.rs
  • src-tauri/src/application/command_bus.rs
  • src-tauri/src/application/commands/mod.rs
  • src-tauri/src/application/commands/toggle_clipboard.rs
  • src-tauri/src/application/commands/update_config.rs
  • src-tauri/src/domain/event.rs
  • src-tauri/src/domain/model/config.rs
  • src-tauri/src/domain/ports/driven/tests.rs
  • src-tauri/src/lib.rs
  • src/components/__tests__/ClipboardIndicator.test.tsx
  • src/stores/__tests__/settingsStore.test.ts
  • src/stores/settingsStore.ts
  • src/types/settings.ts
  • src/views/SettingsView.tsx
  • src/views/SettingsView/AppearanceSection.tsx
  • src/views/SettingsView/BrowserSection.tsx
  • src/views/SettingsView/DownloadsSection.tsx
  • src/views/SettingsView/GeneralSection.tsx
  • src/views/SettingsView/NetworkSection.tsx
  • src/views/SettingsView/RemoteAccessSection.tsx
  • src/views/SettingsView/SettingField.tsx
  • src/views/SettingsView/SettingsView.tsx
  • src/views/SettingsView/__tests__/Sections.test.tsx
  • src/views/SettingsView/__tests__/SettingsView.test.tsx
  • src/views/SettingsView/index.ts
💤 Files with no reviewable changes (1)
  • src/views/SettingsView.tsx

Comment on lines +377 to +382
// Remote Access
pub web_interface_enabled: bool,
pub web_interface_port: u16,
pub rest_api_enabled: bool,
pub api_key: String,
pub websocket_enabled: bool,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Avoid returning the raw API key to the renderer.

Because settings_get and settings_update serialize SettingsDto, the REST API credential gets copied into frontend state/query caches and renderer-visible diagnostics. Prefer a masked value or hasApiKey flag here, with a separate explicit reveal/regenerate/copy flow for the real secret.

Also applies to: 421-423, 525-546

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src-tauri/src/adapters/driving/tauri_ipc.rs` around lines 377 - 382, The
SettingsDto currently includes the raw api_key field (see fields
web_interface_enabled/web_interface_port/rest_api_enabled/api_key/websocket_enabled)
and settings_get/settings_update serialize and return it to the renderer; change
SettingsDto so it does NOT expose the secret directly: remove or replace the
api_key string with a masked placeholder (e.g. "*****") or a boolean hasApiKey
flag, and update any code paths in settings_get and settings_update (and related
code around the other occurrences you flagged) to return the masked/flagged
value instead; implement a separate explicit IPC command (e.g. reveal_api_key /
regenerate_api_key / copy_api_key) that performs secure server-side
retrieval/regeneration and only returns the real secret when authorized.

Comment on lines +82 to +83
settings_get,
settings_update,
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "1) Check whether AppState is managed in src-tauri/src/lib.rs"
rg -n "app\\.manage\\(" src-tauri/src/lib.rs || true

echo
echo "2) Confirm settings commands are registered in generate_handler!"
rg -n "settings_get|settings_update|clipboard_toggle|clipboard_state" src-tauri/src/lib.rs -C2

Repository: mpiton/vortex

Length of output: 1401


AppState must be managed before settings IPC handlers can function.

Lines 82–83 register settings_get and settings_update handlers, but the TODO comment on lines 56–58 shows that app.manage(state) is not yet implemented. These handlers depend on State<'_, AppState> and will fail at runtime until the app state is properly constructed and managed.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src-tauri/src/lib.rs` around lines 82 - 83, Create and manage the AppState
instance before registering the settings IPC handlers: instantiate your AppState
(ensuring it meets Send + Sync requirements or is wrapped in Arc/Mutex as
needed) and call the builder's manage method (app.manage(state) /
builder.manage(state)) prior to registering settings_get and settings_update so
the handlers receiving State<'_, AppState> have a valid managed state at
runtime; update the init sequence to construct the state and call manage(...)
before the handler registration that references settings_get and
settings_update.

- Add Mutex to TomlConfigStore for concurrency-safe read-modify-write
- Fix IPC argument mismatch: wrap settings_update args in { patch: ... }
- Buffer numeric/text inputs locally, commit on blur (no keystroke spam)
- Convert BrowserSection textareas to controlled inputs
- Disable Browse button until directory picker is wired
- Add aria-pressed to accent color buttons for accessibility
- Add htmlFor/id association on SettingToggle and SettingNumberInput
- Separate loading and error states in SettingsView
- Add tauri_bridge test for SettingsUpdated event mapping
- Assert exact 'settings-updated' event name in SettingsView test
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/stores/settingsStore.ts (1)

22-33: ⚠️ Potential issue | 🟠 Major

Optimistic update is not reverted on failure.

If settings_update fails, Line 25’s optimistic config change remains in state, so UI can show unsaved values until a later refresh.

Suggested fix (rollback on failed mutation)
   updateConfig: async (partial) => {
-    set({ isLoading: true, error: null });
-    set((s) => ({
-      config: s.config ? { ...s.config, ...partial } : s.config,
-    }));
+    let previousConfig: AppConfig | null = null;
+    set((s) => {
+      previousConfig = s.config;
+      return {
+        isLoading: true,
+        error: null,
+        config: s.config ? { ...s.config, ...partial } : s.config,
+      };
+    });
     try {
       await tauriInvoke('settings_update', { patch: partial });
     } catch (err) {
       const message = err instanceof Error ? err.message : String(err);
-      set({ error: message });
+      set({ error: message, config: previousConfig });
     } finally {
       set({ isLoading: false });
     }
   },
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/stores/settingsStore.ts` around lines 22 - 33, The optimistic update in
updateConfig mutates state before the remote call and never rolls back on error;
modify updateConfig to snapshot the previous config (e.g., const prev =
get().config) before set(...), then if tauriInvoke('settings_update', { patch:
partial }) throws revert to the snapshot via set({ config: prev }) and still set
the error message, and keep the isLoading toggles as now; ensure you reference
the updateConfig function and the 'settings_update' invoke call when
implementing the rollback.
🧹 Nitpick comments (1)
src/views/SettingsView/RemoteAccessSection.tsx (1)

102-107: Add a confirmation step before regenerating the API key.

This action is immediate and destructive for existing clients; accidental clicks can break integrations.

Suggested guardrail
             <Button
               variant="outline"
               size="icon"
               aria-label="Regenerate API key"
-              onClick={() => handleChange('apiKey', crypto.randomUUID())}
+              onClick={() => {
+                const confirmed = window.confirm(
+                  'Regenerate API key? Existing API clients will stop working until updated.'
+                );
+                if (confirmed) handleChange('apiKey', crypto.randomUUID());
+              }}
             >
               <RefreshCw className="size-4" />
             </Button>
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/views/SettingsView/RemoteAccessSection.tsx` around lines 102 - 107, Add a
confirmation step before regenerating the API key by wrapping the Button's
onClick handler (the Regenerate API key button that currently calls
handleChange('apiKey', crypto.randomUUID())) with a user confirmation flow: show
a confirmation dialog/modal (or call window.confirm) asking the user to confirm
the destructive action, and only invoke handleChange('apiKey',
crypto.randomUUID()) if the user confirms; keep the dialog message clear about
breaking existing clients and optionally provide a cancel path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/views/SettingsView/AppearanceSection.tsx`:
- Around line 19-21: Update the user-facing locale labels in the
AppearanceSection component: replace the 'label' string for the French locale
from "Francais" to "Français" and for the Spanish locale from "Espanol" to
"Español" so the displayed names use correct diacritics; locate the array of
locale objects in AppearanceSection (the entries with value 'fr' and 'es') and
update their label properties accordingly.

In `@src/views/SettingsView/DownloadsSection.tsx`:
- Around line 46-50: The label "Speed limit (MB/s)" and the onChange conversion
in the DownloadsSection (value speedLimitMb, onChange ->
handleChange('speedLimitBytesPerSec', ...)) are inconsistent: the code
multiplies by 1,048,576 (MiB) but the label says MB; update either the label to
"MiB/s" to match the binary conversion or change the multiplier to 1_000_000 to
use decimal MB, and keep the conversion logic otherwise unchanged so
handleChange('speedLimitBytesPerSec', ...) receives the correct bytes/sec.

---

Outside diff comments:
In `@src/stores/settingsStore.ts`:
- Around line 22-33: The optimistic update in updateConfig mutates state before
the remote call and never rolls back on error; modify updateConfig to snapshot
the previous config (e.g., const prev = get().config) before set(...), then if
tauriInvoke('settings_update', { patch: partial }) throws revert to the snapshot
via set({ config: prev }) and still set the error message, and keep the
isLoading toggles as now; ensure you reference the updateConfig function and the
'settings_update' invoke call when implementing the rollback.

---

Nitpick comments:
In `@src/views/SettingsView/RemoteAccessSection.tsx`:
- Around line 102-107: Add a confirmation step before regenerating the API key
by wrapping the Button's onClick handler (the Regenerate API key button that
currently calls handleChange('apiKey', crypto.randomUUID())) with a user
confirmation flow: show a confirmation dialog/modal (or call window.confirm)
asking the user to confirm the destructive action, and only invoke
handleChange('apiKey', crypto.randomUUID()) if the user confirms; keep the
dialog message clear about breaking existing clients and optionally provide a
cancel path.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: bb57544c-d9cd-4245-be87-f2f434161f10

📥 Commits

Reviewing files that changed from the base of the PR and between 6ff5002 and 960b706.

📒 Files selected for processing (13)
  • src-tauri/src/adapters/driven/config/toml_config_store.rs
  • src-tauri/src/adapters/driven/event/tauri_bridge.rs
  • src/stores/__tests__/settingsStore.test.ts
  • src/stores/settingsStore.ts
  • src/views/SettingsView/AppearanceSection.tsx
  • src/views/SettingsView/BrowserSection.tsx
  • src/views/SettingsView/DownloadsSection.tsx
  • src/views/SettingsView/GeneralSection.tsx
  • src/views/SettingsView/NetworkSection.tsx
  • src/views/SettingsView/RemoteAccessSection.tsx
  • src/views/SettingsView/SettingField.tsx
  • src/views/SettingsView/SettingsView.tsx
  • src/views/SettingsView/__tests__/SettingsView.test.tsx
✅ Files skipped from review due to trivial changes (1)
  • src-tauri/src/adapters/driven/event/tauri_bridge.rs
🚧 Files skipped from review as they are similar to previous changes (6)
  • src/stores/tests/settingsStore.test.ts
  • src/views/SettingsView/tests/SettingsView.test.tsx
  • src/views/SettingsView/SettingsView.tsx
  • src/views/SettingsView/BrowserSection.tsx
  • src/views/SettingsView/NetworkSection.tsx
  • src/views/SettingsView/SettingField.tsx

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2 issues found across 13 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src-tauri/src/adapters/driven/config/toml_config_store.rs">

<violation number="1" location="src-tauri/src/adapters/driven/config/toml_config_store.rs:68">
P2: The new mutex only guards `update_config`, but `get_config` can also write `config.toml` without that lock. Concurrent calls can still race and overwrite updates.</violation>
</file>

<file name="src/views/SettingsView/SettingField.tsx">

<violation number="1" location="src/views/SettingsView/SettingField.tsx:55">
P2: After clamping, the local input state is not updated, so the field can keep showing an invalid/out-of-range number when parent value doesn't change.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

- Extend Mutex to get_config for full concurrency safety
- Validate post-merge config instead of patch-only (cross-field: proxy_url
  required when proxy_type != none)
- Update localValue after clamping to avoid stale display
- Fix locale labels: Français, Español, 日本語, 中文
- Clarify speed limit label: MB/s → MiB/s (matches 1048576 conversion)
- Add cross-field validation tests (proxy with/without URL)
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (1)
src/views/SettingsView/SettingField.tsx (1)

17-20: ⚠️ Potential issue | 🟡 Minor

Associate descriptions with controls (aria-describedby) for screen readers.

Both components render description text, but the control never references it. Add a description id and wire it to the control so assistive tech reads the helper text.

Suggested fix
 export function SettingToggle({ label, description, checked, onCheckedChange }: SettingToggleProps) {
   const id = useId();
+  const descriptionId = description ? `${id}-description` : undefined;
   return (
     <div className="flex items-center justify-between gap-4 py-2">
       <div>
         <label htmlFor={id} className="text-sm font-medium">{label}</label>
-        {description && <p className="text-xs text-muted-foreground">{description}</p>}
+        {description && (
+          <p id={descriptionId} className="text-xs text-muted-foreground">{description}</p>
+        )}
       </div>
-      <Switch id={id} checked={checked} onCheckedChange={onCheckedChange} />
+      <Switch
+        id={id}
+        aria-describedby={descriptionId}
+        checked={checked}
+        onCheckedChange={onCheckedChange}
+      />
     </div>
   );
 }
@@
 }: SettingNumberInputProps) {
   const id = useId();
+  const descriptionId = description ? `${id}-description` : undefined;
   const [localValue, setLocalValue] = useState(String(value));
@@
       <div>
         <label htmlFor={id} className="text-sm font-medium">{label}</label>
-        {description && <p className="text-xs text-muted-foreground">{description}</p>}
+        {description && (
+          <p id={descriptionId} className="text-xs text-muted-foreground">{description}</p>
+        )}
       </div>
       <Input
         id={id}
+        aria-describedby={descriptionId}
         type="number"

Also applies to: 65-79

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/views/SettingsView/SettingField.tsx` around lines 17 - 20, The
description paragraph rendered in SettingField (label, description) isn't
referenced by the control, so add an ID for the description (e.g. const descId =
`${id}-description`) and set aria-describedby={description ? descId : undefined}
on the Switch component (the Switch props in SettingField: id, checked,
onCheckedChange) and give the <p> that renders {description} the id={descId};
repeat the same pattern for the other similar block (the second SettingField
instance around the other description rendering) so screen readers can associate
helper text with the control.
🧹 Nitpick comments (1)
src-tauri/src/adapters/driven/config/toml_config_store.rs (1)

27-35: Prefer NotFound handling over exists() pre-checks.

read_or_default() and get_config() both branch on self.path.exists() before doing I/O. That adds redundant syscalls and a cross-process TOCTOU window: another writer can create or remove the file between the check and the read/write. Attempting the read directly and only special-casing ErrorKind::NotFound is more robust. (doc.rust-lang.org)

Also applies to: 64-66

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src-tauri/src/adapters/driven/config/toml_config_store.rs` around lines 27 -
35, The pre-check using self.path.exists() in read_or_default (and similarly in
get_config) should be removed to avoid TOCTOU; instead call
std::fs::read_to_string(&self.path) (or std::fs::read) directly and match the
resulting io::Error: if the error.kind() == io::ErrorKind::NotFound return
Ok(AppConfig::default()) (or appropriate default/None for get_config), otherwise
map the error to DomainError::StorageError as done now; apply the same pattern
when parsing toml (ConfigDto from_str) so only actual NotFound is special-cased
and all other I/O/parsing errors remain mapped to DomainError::StorageError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src-tauri/src/adapters/driven/config/toml_config_store.rs`:
- Around line 44-53: The temp-file write currently uses std::fs::write which
creates the file with platform-default permissions; change the tmp file creation
to use std::fs::OpenOptions with std::os::unix::fs::OpenOptionsExt::mode(0o600)
so the temp config file is created owner-read/write only, then write the TOML
content via that opened file (e.g., open tmp_path with
OpenOptions::new().create(true).write(true).truncate(true).mode(0o600), write
the serialized content, flush, close) and finally rename tmp_path to self.path
as before; update the block around ConfigDto::from(...),
toml::to_string_pretty(...), tmp_path and the rename call to use this secure
OpenOptions-based write.

In `@src/views/SettingsView/AppearanceSection.tsx`:
- Around line 43-57: The Theme and Language Select controls lack programmatic
accessible names; update the SelectTrigger (or the Select component) for the
Theme select (which uses value={config.theme} and onValueChange={(v: ThemeMode)
=> handleChange('theme', v)}) to include an explicit accessible name (e.g., add
aria-label="Theme" or aria-labelledby pointing to its label) and do the same for
the Language select (the other Select block using handleChange('language',
...)). Ensure the aria attribute is applied to the interactive element
(SelectTrigger or the outer Select) so screen readers associate the visible <p>
label with the control.

In `@src/views/SettingsView/SettingField.tsx`:
- Around line 51-56: commit() currently clamps to min/max but can emit values
that don't align to the configured step; adjust the numeric value to the nearest
valid step before clamping/emitting. In the commit function use step (if truthy)
to compute an aligned value by using a base (min ?? 0), computing
Math.round((num - base)/step) * step + base, then clamp that aligned value with
min/max, call setLocalValue(String(alignedClamped)) and
onChange(alignedClamped); keep existing flow and handle step===0/undefined by
falling back to the current behavior.

---

Duplicate comments:
In `@src/views/SettingsView/SettingField.tsx`:
- Around line 17-20: The description paragraph rendered in SettingField (label,
description) isn't referenced by the control, so add an ID for the description
(e.g. const descId = `${id}-description`) and set aria-describedby={description
? descId : undefined} on the Switch component (the Switch props in SettingField:
id, checked, onCheckedChange) and give the <p> that renders {description} the
id={descId}; repeat the same pattern for the other similar block (the second
SettingField instance around the other description rendering) so screen readers
can associate helper text with the control.

---

Nitpick comments:
In `@src-tauri/src/adapters/driven/config/toml_config_store.rs`:
- Around line 27-35: The pre-check using self.path.exists() in read_or_default
(and similarly in get_config) should be removed to avoid TOCTOU; instead call
std::fs::read_to_string(&self.path) (or std::fs::read) directly and match the
resulting io::Error: if the error.kind() == io::ErrorKind::NotFound return
Ok(AppConfig::default()) (or appropriate default/None for get_config), otherwise
map the error to DomainError::StorageError as done now; apply the same pattern
when parsing toml (ConfigDto from_str) so only actual NotFound is special-cased
and all other I/O/parsing errors remain mapped to DomainError::StorageError.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 091fc63c-ce29-49ab-b1b8-6091752a05b0

📥 Commits

Reviewing files that changed from the base of the PR and between 960b706 and c5bccb5.

⛔ Files ignored due to path filters (5)
  • src-tauri/Cargo.lock is excluded by !**/*.lock
  • src-tauri/gen/schemas/acl-manifests.json is excluded by !**/gen/**
  • src-tauri/gen/schemas/capabilities.json is excluded by !**/gen/**
  • src-tauri/gen/schemas/desktop-schema.json is excluded by !**/gen/**
  • src-tauri/gen/schemas/linux-schema.json is excluded by !**/gen/**
📒 Files selected for processing (9)
  • src-tauri/Cargo.toml
  • src-tauri/capabilities/default.json
  • src-tauri/src/adapters/driven/config/toml_config_store.rs
  • src-tauri/src/application/commands/update_config.rs
  • src-tauri/src/lib.rs
  • src/views/SettingsView/AppearanceSection.tsx
  • src/views/SettingsView/DownloadsSection.tsx
  • src/views/SettingsView/SettingField.tsx
  • src/views/SettingsView/__tests__/Sections.test.tsx
✅ Files skipped from review due to trivial changes (2)
  • src-tauri/capabilities/default.json
  • src-tauri/Cargo.toml
🚧 Files skipped from review as they are similar to previous changes (3)
  • src-tauri/src/lib.rs
  • src/views/SettingsView/tests/Sections.test.tsx
  • src-tauri/src/application/commands/update_config.rs

- Set 0o600 permissions on config.toml temp file (owner-only read/write)
- Add aria-label to Theme and Language select triggers
- Respect step when committing numeric values (snap to nearest step)
Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
src-tauri/src/adapters/driven/config/toml_config_store.rs (1)

69-77: Consider sync_all() before rename for durability.

flush() only pushes data to OS buffers; it doesn't guarantee persistence to disk. If the system crashes between flush() and rename(), the temp file content could be lost or incomplete. For truly crash-consistent atomic writes, call sync_all() on the file handle before renaming.

♻️ Optional improvement
             file.write_all(content.as_bytes()).map_err(|e| {
                 DomainError::StorageError(format!("failed to write config tmp file: {e}"))
             })?;
-            file.flush().map_err(|e| {
-                DomainError::StorageError(format!("failed to flush config tmp file: {e}"))
+            file.sync_all().map_err(|e| {
+                DomainError::StorageError(format!("failed to sync config tmp file: {e}"))
             })?;
         }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src-tauri/src/adapters/driven/config/toml_config_store.rs` around lines 69 -
77, The temp-file write path currently calls file.write_all and file.flush but
then immediately uses std::fs::rename(&tmp_path, &self.path); update the
sequence in the function that performs the write (the block using
file.write_all, file.flush, tmp_path and self.path) to call file.sync_all()
after flush and before std::fs::rename, and map any sync_all() error into the
same DomainError::StorageError pattern so the rename only happens after the file
is durably persisted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src-tauri/src/adapters/driven/config/toml_config_store.rs`:
- Around line 69-77: The temp-file write path currently calls file.write_all and
file.flush but then immediately uses std::fs::rename(&tmp_path, &self.path);
update the sequence in the function that performs the write (the block using
file.write_all, file.flush, tmp_path and self.path) to call file.sync_all()
after flush and before std::fs::rename, and map any sync_all() error into the
same DomainError::StorageError pattern so the rename only happens after the file
is durably persisted.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: a1b0b7c7-f53a-4d7b-9591-a96b5a2c0edd

📥 Commits

Reviewing files that changed from the base of the PR and between c5bccb5 and e1401de.

📒 Files selected for processing (3)
  • src-tauri/src/adapters/driven/config/toml_config_store.rs
  • src/views/SettingsView/AppearanceSection.tsx
  • src/views/SettingsView/SettingField.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/views/SettingsView/AppearanceSection.tsx
  • src/views/SettingsView/SettingField.tsx

Copy link
Copy Markdown

@cubic-dev-ai cubic-dev-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 3 files (changes from recent commits).

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="src-tauri/src/adapters/driven/config/toml_config_store.rs">

<violation number="1" location="src-tauri/src/adapters/driven/config/toml_config_store.rs:72">
P2: `flush()` does not make file writes durable for `File`; use `sync_all()` before rename to preserve atomic-write guarantees across crashes.</violation>
</file>

Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.

mpiton added 3 commits April 11, 2026 14:16
The plugin uses #[cfg(unix)] for its server module, so it can't compile
on Windows. Gate both the Cargo dependency and the plugin registration
on cfg(unix).
Tauri validates all capability permissions at build time regardless of
whether the plugin is loaded. Move pilot:default to a separate
dev-pilot.json capability file restricted to linux/macOS platforms.
@mpiton mpiton merged commit 8eeaa25 into main Apr 11, 2026
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation frontend rust ui

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant